Skip to content

fix: clear the warning-sink ThreadLocal and drop dead test state - #146

Merged
svaningelgem merged 2 commits into
masterfrom
fix/sonar-master-findings
Aug 6, 2026
Merged

fix: clear the warning-sink ThreadLocal and drop dead test state#146
svaningelgem merged 2 commits into
masterfrom
fix/sonar-master-findings

Conversation

@svaningelgem

Copy link
Copy Markdown
Collaborator

Fixes the 5 Sonar issues still open on master after the setAccessible ones were accepted. 263 tests green, branch coverage unchanged at 303/303.

The real one

TypeConverter:31Call "remove()" on "ACTIVE"

ACTIVE was a ThreadLocal created with withInitial(...) and only ever set, never removed. YamlFileInterface.load(Object) installs the plugin's logger and restores the previous value in a finally — but "restoring" wrote the default back rather than clearing the entry, so every thread that ever loaded a config kept a sink referenced for its lifetime. On a server thread pool those threads outlive the load.

It now starts unset, warn() falls back to LOG when nothing is installed, and restoring a null sink calls remove():

static @Nullable Consumer<String> pushSink(final @Nullable Consumer<String> sink) {
    Consumer<String> prev = ACTIVE.get();
    if (sink == null) {
        ACTIVE.remove();
    } else {
        ACTIVE.set(sink);
    }
    return prev;
}

The existing finally { pushSink(prev) } call site needed no change — prev is now null on the first install, which clears the entry.

Dead state removed

  • LeniencyTest:7 — unused java.nio.file.Files import.
  • NamespacedKey:7 — the ns field was assigned and never read. The double exists so tryFormatAsKeyed can call key() then value(); the namespace was never part of that. Field and constructor parameter dropped, and the single call site in SoundImpl updated.
  • Sounds:9 — same story. The tests only compare identity (Sounds.MY_SOUND_ROCKS), so the name field, the getSound factory and the @NotNull import all went. The class deliberately keeps no public String constructor and no toString() override — that is what makes YamlWriter fall through to static-field matching — so the comment now records it.

Kept on purpose

StaticFieldTestClass:11PRIVATE_INSTANCE

This one is a false positive worth explaining rather than obeying. The field is read only by reflection, from YamlWriter#getStaticFieldName, and it is the only case exercising the field.canAccess(null) == false branch.

Measured rather than assumed — deleting it:

getStaticFieldName BRANCH  covered=7 missed=1     (was 8/8)

So it stays, with @SuppressWarnings("unused") and a comment recording why, which is also what stops the issue reappearing.

On the coverage numbers

A local build shows 2 uncovered lines in YamlWrapperFactory (the v1-vs-v2 selection). That is not a regression and not related to this change: only one SnakeYAML version can take that branch per run. Verified the union across report-1.14.xml and report-2.4.xml leaves no uncovered lines, which is why Sonar — aggregating all four reports — reports 100%.

TypeConverter's ACTIVE ThreadLocal was never removed, so the last sink stayed
referenced for the life of the thread -- a slow leak on a server thread pool. It now
starts unset, warn() falls back to LOG, and restoring a null sink clears the entry.

Also removes an unused import, the unused namespace on the NamespacedKey test double,
and the unused name on Sounds. StaticFieldTestClass.PRIVATE_INSTANCE is kept and
annotated instead: it is read only by reflection and is the sole case covering the
canAccess(null) == false branch, so removing it drops getStaticFieldName to 7/8.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/main/java/org/avarion/yaml/TypeConverter.java 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

NamespacedKey takes a namespace and a key because the real Bukkit class does; the
namespace is now exposed through getNamespace() the same way, rather than the field
being deleted for being unread. Sounds likewise keeps its named instances and static
factory, with the getName() accessor StaticFieldTestClass already uses.

Fidelity to the API under reflection is the point of these doubles, so unused-field
warnings on them are answered by using the field, not by reshaping the class.
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

@svaningelgem
svaningelgem merged commit 6ecc4dd into master Aug 6, 2026
3 checks passed
@svaningelgem
svaningelgem deleted the fix/sonar-master-findings branch August 6, 2026 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants